Conversation
The goal here is to make "nj_max" accessible at the level of the controlling Python script. The approach is to add a Sapporo Light function that will return "nj_max", next add a PH4 function that can call this function and, subsequentlty, provide a Python interface to this function, i.e. a layered approach.
According to the AMUSE guideline https://amuse.readthedocs.io/en/latest/tutorial/legacy_code.html we should be able to leave "src/amuse_ph4/interface.cc" untouched, i.e. exactly the same as in the "main" branch. I.e., this code should not be necessary.
In "sapporoG6lib.cpp" we can do "return grav.get_nj_max()" since that has "sapporo grav;".
This compiles, i.e.
"./setup install sapporo_light" and "./setup install amuse-ph4" will complete without error.
However, Erwan's script with an additional "print(f"{code.get_nj_max() = }, ")" will yield
"
TypeError: ph4Interface.get_nj_max() takes 0 positional arguments but 1 was given
"
HannoSpreeuw
added
kind: feature request
LourensVeen
left a comment
LourensVeen
left a comment
There was a problem hiding this comment.
This should fix the 0 positional argoments error at least.
This function definition does not need to be separated.
1) 'extern "C"' not needed here 2) We need to adhere to AMUSE standards, I guess the legacy decorator implies that we need to set an argument 'int * value' and '*value = jd->get_nj_max(); return 0;' or we will be returned an empty list instead of an integer. 3) As a consequence of 2) we need to add an argument to the function: 'function.addParameter(...'.
"interface.cc" should have been added as part of the previous commit.
Essential is the '#ifdef GPU', since, when one executes "./setup install amuse-ph4" one will not link with Sapporo as one would when executing "./setup install amuse-ph4-sapporo". So "GPU" and "Sapporo" go hand in hand and one needs to make sure that "./setup install amuse-ph4" will complete without error when no GPU is available.
|
@LourensVeen with these recent commits, this will build and run, i,.e. if one adds a line to Erwan's script after the line it will print 131072, which means that the user gets to know the maximum number of stars that Sapporo Light allows for and adjust his/her Python script accordingly. Closes #1208 |
In the sense that the user has access to But when running a simulation with |
LourensVeen
left a comment
LourensVeen
left a comment
There was a problem hiding this comment.
Looks good, but I have a few small changes to request. Thanks!
lib/sapporo_light/sapporo.h
Outdated
|
|
||
|
|
||
| }; | ||
| int get_nj_max() const; |
There was a problem hiding this comment.
This works fine, but maybe it belongs with get_n_pipes on line 191?
There was a problem hiding this comment.
Okay, you mean that would be a better place in terms of the program's logic?
lib/sapporo_light/sapporo.h
Outdated
| }; | ||
|
|
||
| #endif | ||
|
|
There was a problem hiding this comment.
This should be above the #endif on 232, or things will go wrong when there's a header include loop. Maybe just add it on line 104 inside the existing extern "C" block there?
There was a problem hiding this comment.
Right, will do.
src/amuse_ph4/src/jdata.cc
Outdated
| #ifdef GPU | ||
| return g6_get_nj_max_(); | ||
| #else | ||
| return 0; |
There was a problem hiding this comment.
So on the CPU we support no particles at all? I think it makes sense to put a large number here, like INT_MAX from limits.h or, since it's C++, std::numeric_limits<int>::max() from limits. That way a script that checks won't always crash in CPU mode, only when it runs out of memory.
There was a problem hiding this comment.
Ah yeah, that makes sense. I will add that.
Following@LourensVeen recommendation to prevent things from going wrong when there's a header include loop.
Following @LourensVeen's recommendation: When running PH4 simulations on a CPU instead of a GPU, Sapporo Light's "nj_max" does not play a role, so one will want to return a different very large number indicating the maximum number of stars allowed for the simulation. In case the number of stars in a simulation running on a CPU will approach "nj_max = std::numeric_limits<int>::max();" one will probably run into a memory error, but that's another matter.
2 participants
Trying to fix #1208, but not there yet, i.e. this will compile but not display a meaningful error message like "Hey, you have inserted too many stars, with respect to the Sapporo Light limit of$2^{17}=131,072$ ".
More specifically, Erwan's script will run flawlessly with$<2^{17}$ stars, but yield
with$2^{17} + 1$ stars.
This MR aims to propagate nj_max to "higher levels", i.e. to PH4 such that the controlling Python script - Erwan's script - can access it.
Adding a line$131072$ but instead yields
print(f"{code.get_nj_max() = }, ")to that script should yieldI have tried to work around this, but I have not succeeded.
Perhaps @LourensVeen could shed some light on this?